-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: move "Troubleshoot" out of About #39230
Conversation
@alitoshmatov Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@danieldoglas In main branch, the text is "Test Preferences". In new design, we use "Testing Preferences". What should we use? |
@tienifr please use the text from the new design |
I just asked for the translations in slack. Once all the translations are added, you can review PR @alitoshmatov |
@danieldoglas Can you help check this slack thread? |
@alitoshmatov I think you can review this PR while the internal team fulfills the translations |
@alitoshmatov All translations are added |
@shawnborton I fixed comment |
@tienifr subtitle styles are not matching. Also, I think the alignment is a bit off, I think it should match subtitles in other pages, for example about page. |
Good catch on this, let's fix. Again, take a look at the other places in Settings like Security and About for inspiration on correct implementation. |
This comment has been minimized.
This comment has been minimized.
This is looking pretty good. I think the illustration in the header needs to be a bit bigger and centered: And I ran into some weirdness with the nav on smaller viewports. Maybe that's unrelated to this, I'm not sure, so I thought I'd call it out:
CleanShot.2024-04-05.at.08.19.35.mp4 |
Oh sorry. I misunderstood @dannymcclain 's thought
We can increase the size of the inner lottie by update these props, but I think it will be better if we supply a new lottie file |
Cool, I think we might be fine if we just do my suggestion above here - can you please try that and report back? Thanks! |
@shawnborton I fixed the issue "the illustration in the header needs to be centered". Below is result: |
@shawnborton I just merged main, and there is a new option, named "Save the world". So what is the correct order when displaying these options? Currently this PR, is: |
That order looks good to me. |
This also looks good to me! I'd say we're ready for final review then. Thanks for the adjustments! |
<Text style={[styles.textNormal, subtitleMuted && styles.colorMuted]}>{subtitle}</Text> | ||
</View> | ||
)} | ||
{renderSubtitle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Did you test the submit a bug link? It doesn't work for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I tested and It works well:
Screen.Recording.2024-04-09.at.18.50.59.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks for confirming.
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
@shawnborton I fixed this. Result: |
Thanks! |
@alitoshmatov can you please give this a review? Thanks! |
@alitoshmatov Please help review PR once you have a chance |
Reviewing |
Reviewer Checklist
Screenshots/VideosAndroid: Nativetroubleshooting-android.movAndroid: mWeb Chrometroubleshooting-mweb.moviOS: Nativetroubleshooting-ios.mp4iOS: mWeb Safaritroubleshooting-safari.mp4MacOS: Chrome / Safaritroubleshooting-web.movMacOS: Desktoptroubleshooting-desktop.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/danieldoglas in version: 1.4.63-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.63-21 🚀
|
Details
Fixed Issues
$ #38594
PROPOSAL: #38594 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-03-29.at.18.28.10.mov
Android: mWeb Chrome
Screen.Recording.2024-03-29.at.18.30.36.mov
iOS: Native
Screen.Recording.2024-04-01.at.14.40.37.mov
iOS: mWeb Safari
Screen.Recording.2024-03-29.at.18.32.18.mov
MacOS: Chrome / Safari
Screen.Recording.2024-03-29.at.18.15.02.mov
MacOS: Desktop
Screen.Recording.2024-03-29.at.18.22.17.mov